Skip to content

feat: adopt sqlx::migrate for PostgreSQL catalog and data migrations (ADR-0003) - #221

Open
yesyayen wants to merge 8 commits into
ExtendDB:mainfrom
yesyayen:feat/sqlx-migrate
Open

feat: adopt sqlx::migrate for PostgreSQL catalog and data migrations (ADR-0003)#221
yesyayen wants to merge 8 commits into
ExtendDB:mainfrom
yesyayen:feat/sqlx-migrate

Conversation

@yesyayen

@yesyayen yesyayen commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

What

Replace the homegrown PostgreSQL migration runner with sqlx's built-in migrator
(sqlx::migrate!) for both the catalog and data databases. sqlx checksums each
migration file and refuses to run when an applied file changed. extenddb migrate
now runs both migrators; the catalog_version write moves to a separate step
after the catalog migrator runs. Deletes the schema_history table and the
homegrown runner. Bumps CATALOG_VERSION 0.0.2 -> 0.1.0.

Also adds a stateless unit test that pins each shipped migration's checksum, so
editing an already-shipped (immutable) migration .sql fails the CI test job,
catching the edit at PR time, not just at runtime.

Why

The old runner tracked migrations by filename with no checksum, so editing an
already-shipped migration was a silent no-op that caused schema drift (the
incident behind ADR-0003). sqlx's _sqlx_migrations table closes that gap.

Closes # n/a (tracked by ADR-0003)

Testing done

  • cargo build, cargo fmt --all -- --check, cargo clippy --all-targets -- -D warnings,
    cargo test --workspace all green. Tripwire pins migration counts + CATALOG_VERSION.
  • CI net: migration_checksums_are_pinned pins each migration's SHA-384 checksum.
    Editing an already-shipped migration file changes its checksum and fails
    cargo test --workspace (the GitHub PR test runner, no database required), so an
    edit to an immutable migration is caught in CI, not only at runtime. Verified by
    editing a migration and observing the test flip to FAILED, then reverting.
  • Full lifecycle against local PostgreSQL: init, serve (version gate passes),
    idempotent migrate, version-gate rejection + migrate repair, editing an applied
    migration -> loud checksum failure ("migration 2 was previously applied but has
    been modified"), destroy + re-init. _sqlx_migrations created in both databases;
    schema_history gone.

Review follow-up

Addressed external review of the branch:

  • Pre-sqlx catalog guard (was the one merge-blocking item). A catalog from the
    old runner (has schema_history, no _sqlx_migrations) cannot be adopted in place:
    re-running 001 fails on a non-idempotent CREATE INDEX. Per ADR-0003 the upgrade
    path is destroy + init, so migrate now detects a pre-sqlx catalog up front and
    refuses with that directive (verified: it bails before touching anything, version
    untouched), instead of failing later on cryptic DDL. Added a CLI lifecycle test for it.
  • Reconciled the admin guide's version-mismatch remedy with the upgrade manual
    (destroy + init for the 0.1.0 sqlx adoption).
  • Added the pre-1.0 semver note to the upgrade manual's Version Semantics section.
  • Restored the .sql suffix on the pending-migration report; noted the checksum test's
    coupling to sqlx's SHA-384 algorithm.

Checklist

  • I have read CONTRIBUTING.md
  • All tests pass (cargo test --workspace)
  • Code is formatted (cargo fmt --check)
  • Clippy is clean (cargo clippy -- -W clippy::pedantic)
  • I have added or updated tests for new functionality
  • I have updated documentation if behavior changed
  • Breaking changes are noted below

ADR / RFC: docs/adr/0003-catalog-migration-mechanism.md

Breaking changes

Upgrading from a pre-sqlx catalog requires destroy + init, which drops both
databases (all items wiped, not just schema). Acceptable at v0.1 with no dependent
catalogs. CATALOG_VERSION 0.0.2 -> 0.1.0. Documented in the upgrade manual.

@yesyayen yesyayen changed the title Adopt sqlx::migrate for PostgreSQL catalog and data migrations (ADR-0003) feat: adopt sqlx::migrate for PostgreSQL catalog and data migrations (ADR-0003) Jul 22, 2026
yesyayen added 8 commits July 28, 2026 13:26
Replace the homegrown filename-tracked runner with sqlx's migrator for both
the catalog and data databases (ADR-0003). Each database tracks applied
migrations, with per-file checksums, in _sqlx_migrations; editing an applied
migration is now a hard error instead of a silent no-op.

- Enable the sqlx `migrate` feature.
- run_catalog_migrations / run_data_migrations call sqlx::migrate!().run().
- Delete schema_history DDL, CATALOG_MIGRATIONS/DATA_MIGRATIONS, is_migration_applied,
  record_migration; drop BEGIN/COMMIT from migration files (sqlx wraps each in a txn).
- Rehome the catalog_version write to a separate step after the catalog migrator runs.
- migrate runs both migrators unconditionally so checksum validation always fires.
- Bump CATALOG_VERSION to 0.1.0 (breaking: existing catalogs re-init).
- Add .gitattributes pinning *.sql to LF; update the upgrade manual.

Signed-off-by: Anandh Somasundaram <yesyayen@gmail.com>
Add a tripwire pinning the embedded migration counts and CATALOG_VERSION
(ADR-0003). Rewrite the two CLI lifecycle tests that queried the removed
schema_history table to assert on sqlx's _sqlx_migrations ledger.

Signed-off-by: Anandh Somasundaram <yesyayen@gmail.com>
…migrations

A stateless unit test pins each shipped migration's sqlx checksum, so
`cargo test` (the PR runner, no database) fails if an already-applied
migration file is edited. This catches in CI what sqlx otherwise only
enforces at runtime against a live catalog. ADR-0003.

Signed-off-by: Anandh Somasundaram <yesyayen@gmail.com>
A catalog created by the old filename-tracked runner (has schema_history,
no _sqlx_migrations) cannot be adopted in place: re-running 001 fails on a
non-idempotent CREATE INDEX. Per ADR-0003 the upgrade path is destroy + init,
so `migrate` now detects a pre-sqlx catalog up front and refuses with that
directive instead of failing later on cryptic DDL. Aligns the admin guide with
the upgrade manual, notes pre-1.0 semver, and restores the .sql suffix on the
pending-migration report.

Signed-off-by: Anandh Somasundaram <yesyayen@gmail.com>
…ective

Signed-off-by: Anandh Somasundaram <yesyayen@gmail.com>
…s changelog

Transactional migrations roll back completely on a mid-apply crash (no dirty
row); re-running migrate retries. Dirty state only applies to -- no-transaction
migrations, of which there are none. Also note Version History is the changelog.

Signed-off-by: Anandh Somasundaram <yesyayen@gmail.com>
…guard

The ADR's operational note said a mid-apply crash leaves a dirty migration;
for transactional migrations sqlx rolls back fully (no dirty row) and re-run
retries. Also note why the pre-sqlx guard checks only the catalog.

Signed-off-by: Anandh Somasundaram <yesyayen@gmail.com>
After a refused migrate, _sqlx_migrations must not exist, proving the guard
fired before sqlx could create its ledger.

Signed-off-by: Anandh Somasundaram <yesyayen@gmail.com>
@yesyayen
yesyayen force-pushed the feat/sqlx-migrate branch from b353fd8 to ca881a4 Compare July 28, 2026 20:26
robinnsc added a commit that referenced this pull request Aug 4, 2026
…isory lock

Two replicas running `extenddb migrate` at once race each other: both evaluate
which migrations are pending before either records anything, both apply, and
one fails with a duplicate pg_type_typname_nsp_index key when the concurrent
CREATE TABLE IF NOT EXISTS statements collide in PostgreSQL's system catalog.
That makes an idempotent container entrypoint, which runs migrate on every
start of every replica, unsafe.

Take a namespaced session-level advisory lock around the migration step so
migrators serialize. The second blocks, then finds the schema already applied
and no-ops. The lock is held on a dedicated connection to the catalog database
for the duration and released on every path, including the "nothing to do"
early return and error returns; if the process dies, PostgreSQL releases it
when the connection closes.

- A migrator that must wait tries the lock first and prints why it is waiting,
  instead of sitting silent for as long as the other migration takes.
- Acquiring is not re-entrant: a second acquire would open a second connection
  and block on the lock the first holds, deadlocking against itself, so it is
  rejected. A failed unlock is reported rather than swallowed, though closing
  the connection releases the lock anyway so it is never fatal.
- Advisory locks are scoped to a database, so migrators serialize only if they
  share a catalog database — they do, since it comes from the same connection
  string.
- After acquiring, verify against pg_locks that this session holds the lock,
  and fail hard if it does not. A transaction-pooling proxy (pgbouncer in
  transaction mode, RDS Proxy) puts the lock on an arbitrary pooled backend,
  which otherwise leaves migrators unserialized while looking safe. One
  statement maps to one backend even through such a proxy, so the check detects
  it. Direct RDS and Aurora connections pass.
- init takes the same lock around its own schema work, so it cannot race a
  migrate running on another replica. Two concurrent inits cannot reach the
  migrations at all: the second aborts earlier at create_catalog_db because the
  database already exists, so this guards the narrower init-versus-migrate
  overlap.
- The Bootstrapper trait gains acquire_migration_lock and
  release_migration_lock with default no-ops, so out-of-tree backends compile
  unchanged. A new MinimalBootstrapper test implements only the required
  methods, so it stops compiling if a defaulted method loses its default, and
  pins object safety.
- tests: add tests/test_cli_migrate_concurrency.py asserting that two
  concurrent `migrate --yes` runs both succeed with exactly one applying the
  migration and the other observing it as done, and that a migrate blocked on a
  lock held from an external session waits rather than proceeding, then
  completes and reports that it waited. Both tests fail when the lock is
  disabled.
- devtools/run-tests: exclude the new file from the main pytest suite and run
  it in the CLI section instead, alongside test_cli_lifecycle.py. Like those
  tests it starts and stops its own servers and creates its own databases, so
  it cannot run in parallel against the shared instance the main suite uses.

Rebased onto the post-#218 layout: cmd_migrate and cmd_init now live in
crates/app. The guard is unaffected by that move and by the registry removal;
sqlx::migrate (#221) is not yet adopted, so the custom migration runner this
serializes is still in place.
robinnsc added a commit that referenced this pull request Aug 6, 2026
Two replicas running `extenddb migrate` can both evaluate pending work
before either records it, then apply the same migration concurrently.
PostgreSQL can reject the duplicate DDL in its system catalogs, making
an otherwise idempotent container startup unsafe.

Serialize the complete migration decision and apply sequence with a
namespaced PostgreSQL transaction-level advisory lock. The lock is held
by an explicit transaction on a dedicated catalog connection, so it
covers the version read and data-migration pending check, catalog and
data migrations and final ledger observation. A peer acquires the
lock only after the holder finishes, then observes no pending work.

- Try `pg_try_advisory_xact_lock` first so a contending process can
  explain why it is waiting, then use `pg_advisory_xact_lock` for the
  blocking acquisition.
- Keep the explicit transaction open for the full migration interval.
  Transaction-pooling proxies retain one backend until rollback. This
  avoids session locks split across pooled backend sessions.
  Direct PostgreSQL, RDS, Aurora, PgBouncer transaction pooling, and RDS
  Proxy follow the same transaction-scoped lock contract.
- Bound the wait with a five-minute transaction-local `lock_timeout`.
  PostgreSQL SQLSTATE 55P03 becomes an actionable error naming the
  migration advisory lock and explaining that the holder may be wedged.
- Flush waiting and acquired messages so container logs show progress
  while the process is still blocked rather than only after it exits.
- Verify the granted row in `pg_locks`, including the two-integer key
  subtype, ExclusiveLock mode, and granted state. A key or lock-mode
  mismatch fails closed before migrations run.
- Release by rolling back the dedicated transaction, then close the
  connection as a backstop. PostgreSQL also rolls the transaction back
  and releases it when the process or backend dies without cleanup.
- Reject a second in-process acquire instead of deadlocking on a
  new transaction waiting for the lock already held by the first.
- Scope the lock to the catalog database. Replicas sharing that catalog
  serialize even when migration statements touch the data database;
  deployments using different catalogs remain independent.
- Lock init's schema phase too, so init and migrate cannot race. Two
  fresh init processes still do not queue. The second fails earlier
  while creating the already-existing catalog database.
- Add default no-op lock methods to `Bootstrapper`. Existing external
  backends preserve current behavior until they opt into a backend lock.
  A MinimalBootstrapper test pins defaults and trait object safety.

The advisory lock deliberately does not claim to make one migration
atomic with its ledger write. The custom runner still applies SQL and
records the filename in separate commits. Both apply sites carry a TODO
for #221, which removes the migration files' internal BEGIN/COMMIT and
lets sqlx commit each migration with its ledger row.

Current recovery properties are narrower: catalog 001 is normally
shielded by its version write, data 001 has an adoption guard, data 002
is repeatable, and replaying data 003 drops the token table. This
gap must close before another migration relies on atomic recording.

Add deterministic PostgreSQL CLI coverage:

- Hold an uncommitted `schema_history` row so the first migrator commits
  002's DDL and blocks exactly before its ledger insert. Start a second
  migrator and prove it waits. Release the barrier and verify both
  processes succeed, exactly one applies 002, and one ledger row
  exists.
- Assert `gsi_pending` is committed while the 002 ledger row remains
  invisible, pinning the known apply-versus-ledger gap.
- SIGKILL the holder without release, observe the waiting peer acquire
  the lock, then release the ledger barrier and verify recovery. This
  proves lock release on connection death.
- Hold the lock from an external session and prove migrate explains the
  wait before the holder releases it, then completes successfully.

Make subprocess reads unbuffered and cleanup exception-safe so tests
cannot miss Python-buffered data or leak children after failure.
Explicitly flush the Rust messages that the live tests observe.

Integrate main, including container-readiness and SQLite changes.
Resolve the CLI runner overlap as a union: lifecycle,
container-readiness, and migration-concurrency files are excluded from
the shared pytest run
and executed together in the dedicated PostgreSQL CLI section.

Verification on the final tree:

- fmt and clippy `-D warnings` clean
- release build clean
- 669 workspace tests pass, with 3 ignored
- 28 live PostgreSQL CLI tests pass, with 1 Unix-socket skip
- all three migration-concurrency tests pass on PostgreSQL 16.10
- a one-second timeout mutation emits the exact diagnostic in 2.21s;
  the committed value is restored to five minutes and release rebuilt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant